Skip to content

Conversation

@JakobJingleheimer
Copy link
Member

@JakobJingleheimer JakobJingleheimer commented Jan 5, 2025

Description

Validation

Related Issues

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@JakobJingleheimer JakobJingleheimer added content Issues/pr concerning content learn Issues/pr concerning the learn section labels Jan 5, 2025
@JakobJingleheimer JakobJingleheimer requested a review from a team as a code owner January 5, 2025 15:29
@vercel
Copy link

vercel bot commented Jan 5, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Feb 2, 2025 2:05pm

@AugustinMauroy
Copy link
Member

I'm not fan of the provided example because it's based on experimental api.

maybe do same things as this test (do with jest). But I mean based on hand written variable
https://github.com/nodejs/nodejs.org/blob/main/apps/site/util/__tests__/detectOS.test.mjs

@JakobJingleheimer
Copy link
Member Author

JakobJingleheimer commented Jan 5, 2025

Hmm, I think that example isn't quite as good. The one I have is unknowable.

I'm happy to just switch the assertion (and far less work 😅)

EDIT: ohhh! Wait, is the experimental API you're concerned about globSync? (Rather than assert.partialDeepStrictEqual)

@AugustinMauroy
Copy link
Member

EDIT: ohhh! Wait, is the experimental API you're concerned about globSync? (Rather than assert.partialDeepStrictEqual)

Yup and and using a variable is easier because the reader can copy past and run.

@JakobJingleheimer
Copy link
Member Author

using a variable is easier because the reader can copy past and run

A variable for what?

@AugustinMauroy
Copy link
Member

AugustinMauroy commented Jan 12, 2025

A variable for what?

import { isBiscuit } from "factory";

const valid_tests_case = [
  "cookies",
  "speculoos"
];

test("isBiscuit", (t) => {
   for() {
   // loop for dynamic test 
   t.test(`${e} - should be true`)
   }
});

in this case it's use Biscuit because it's fun

But here it's test user agent

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to merge whenever the PR becomes ready.

@JakobJingleheimer
Copy link
Member Author

JakobJingleheimer commented Feb 2, 2025

Hmm, I've given this some more thought, and I think the experimental API used is okay because it does not affect the main issue of the article. Thoughts on that?

How about both options? The user-agent one is simple, and the glob is advanced; I know I frequently see docs cover a simple example but not an advanced one, which leaves an advanced user on their own to figure out something complicated (which is very frustrating).

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ! maybe open an issue after this pr merged to explore the possibility of making one article per use case, as this one is starting to get long.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2025

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟠 89 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 99 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 99 🟢 100 🟢 96 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2025

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 88%
87.1% (736/845) 72.13% (233/323) 86.33% (139/161)

Unit Test Report

Tests Skipped Failures Errors Time
185 0 💤 0 ❌ 0 🔥 5.691s ⏱️

@JakobJingleheimer JakobJingleheimer added this pull request to the merge queue Feb 2, 2025
Merged via the queue into main with commit 837a461 Feb 2, 2025
14 of 16 checks passed
@JakobJingleheimer JakobJingleheimer deleted the learn/add-loop-example-to-test branch February 2, 2025 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

content Issues/pr concerning content learn Issues/pr concerning the learn section

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants